feat(EN-1334): usagebuilder subsystem + event-counter cutover (EN-1420)#1464
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Approve — automated reviewAll blocker and major findings raised during the review cycle have been resolved or formally accepted as design decisions prior to this consolidation pass. The stale-cursor-after-rollback blocker is addressed by No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 8 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1464 (comment)
a705c93 to
5733eb9
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (72.06%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1464 +/- ##
================================================
+ Coverage 74.78% 74.87% +0.09%
================================================
Files 436 445 +9
Lines 46424 47096 +672
================================================
+ Hits 34719 35264 +545
- Misses 8614 8677 +63
- Partials 3091 3155 +64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 8 new inline findings.
Summary: #1464 (comment)
c638a14 to
41c8ba7
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1464 (comment)
41c8ba7 to
4c64ab8
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 11 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
aaa6014 to
cedcab0
Compare
|
Rebased on Addressed the 9 outstanding NumaryBot threads inline; summary:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Found one invariant-level issue.
This PR introduces a new persisted projection store (internal/storage/usagestore) for template usage and the ledger usage counters, but internal/application/check/checker.go does not add any compare pass that re-derives those values from the audit chain and compares them against the stored usagestore rows. The current Check flow still stops at the existing projections (compareVolumes, compareMetadata, compareTransactions, compareExclusionProjections, compareIdempotencyOutcomes, compareIndexes, etc.); rg finds no usagestore / usage-counter / template-usage verification in the checker.
That violates invariant #8 from CLAUDE.md: the audit log is the only source of truth, every other persisted dataset is a projection, and every new persisted projection must be verified by the checker. As written, tampering with PrefixTemplate or PrefixCounter rows can change GetTemplateUsage / GetLedgerStats results without CheckStore surfacing a mismatch. Please add checker coverage that replays the audit-derived usagebuilder state and compares every persisted usage projection, emitting an appropriate CHECK_STORE_ERROR_TYPE_* on divergence.
0649e1a to
18a8629
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1464 (comment)
18a8629 to
1443eb1
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Requesting changes on current head 6f4519f. I validated the current NumaryBot findings against the code and am not duplicating its inline threads.
Blocking issues:
-
Skipped CreateTransaction orders still inflate usage counters.
processAuditEntriesdispatches any audit item with a non-zeroLogSequence, includingOrderSkippedlogs.readLog()returns empty annotations for anOrderSkippedpayload, butdispatchCreateTransactionstill incrementsCounterReference,CounterNumscriptExecution, and template usage directly from the raw order atinternal/application/usagebuilder/process_audit.go:469-491. A skipped transaction should contribute no successful transaction/template usage counters; gate those increments on the produced log being a realCreatedTransaction. -
Skipped RevertTransaction orders still inflate
RevertCount.dispatchRevertTransactionincrementsCounterRevertbefore inspecting the produced log atinternal/application/usagebuilder/process_audit.go:508. If the audit item points at anOrderSkippedlog,readLog()returns no reverted transaction, but the revert counter is already bumped. Only increment after confirming the log payload isRevertedTransaction. -
LedgerStats.metadata_count = 3was removed without reserving the tag/name.misc/proto/common.proto:1647-1655jumps from field 2 to field 4 but does not reserve3or"metadata_count". This is a removed public/protobuf field; please addreserved 3;andreserved "metadata_count";and regenerate.
Non-blocking but should be cleaned: docs/technical/architecture/subsystems/usage/counters.md still opens by saying the page explains how the checker verifies these counters, while the current design explicitly keeps usagestore out of main-store checker scope. Please align that intro with the accepted peer-store/rebuild-health framing.
flemzord review: dispatch* incremented success-scoped usage counters from the raw order even when the produced log was an OrderSkipped (order committed nothing). - readLog now reports the produced log kind (isCreatedTx / isRevertedTx). - dispatchCreateTransaction: gate the reference / numscript-execution / template-usage increments on a produced CreatedTransaction (skips contribute nothing; posting/volume counters already derive from the produced log). - dispatchRevertTransaction: gate CounterRevert on a produced RevertedTransaction (previously bumped unconditionally before reading the log). - docs/usage/counters.md: intro aligned to the peer-store framing (usagestore is out of main-store checker scope; integrity is rebuild-health, not verified by the checker); Revert/Numscript/Reference deltas documented as per-successful-order. go build ./..., usagebuilder tests, golangci-lint — green.
|
@flemzord merci pour la re-review — traité dans 1. CreateTransaction skippé gonflait les compteurs ✅ — 2. RevertTransaction skippé gonflait 3. 4. (non-bloquant)
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Re-checking current head 0510143f.
Thanks, the skipped-order counter bugs and the usagebuilder counters doc issue are fixed in this head.
One blocker remains: LedgerStats.metadata_count = 3 is still removed from misc/proto/common.proto without reserving the field number and name. Current LedgerStats jumps from field 2 to field 4, but public/protobuf removals must reserve both the tag and the name to prevent future incompatible reuse.
Please add:
reserved 3;
reserved "metadata_count";inside message LedgerStats, then regenerate the protobuf outputs.
…usagebuilder # Conflicts: # internal/infra/state/write_set_ephemeral_purge.go # internal/proto/commonpb/common.pb.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Re-reviewing current head ea05119a: not approvable yet.
I revalidated the current diff and agree with the two latest NumaryBot color findings; I am not duplicating those inline threads. I am also not blocking on the LedgerStats.metadata_count reservation request under the current pre-GA/no-compat direction.
Blocking issues:
- The indexbuilder drops
TouchedVolume.colorwhen building purged/ephemeral exclusion keys.
misc/proto/common.proto defines TouchedVolume as (account, asset, color), and internal/application/indexbuilder/process_logs.go later checks exclusions with the full (account, asset, color) tuple. However, internal/application/indexbuilder/applied_proposal_sync.go builds the purged/ephemeral exclusion set using only account and asset. A colored purged/ephemeral bucket therefore does not match the exclusion lookup for that same colored posting, leaving stale account-to-transaction mappings in the readstore; the uncolored bucket for the same account/asset can also be over-excluded.
- The usagebuilder deduplicates per-entry volume annotations without
color.
internal/application/usagebuilder/process_audit.go defines its local volumeSetKey as (ledger, account, asset), then uses it for newKept, purged, and ephemeral volume annotations. Volume identity is now (ledger, account, asset, color), so two colors of the same account/asset in the same audit entry collapse into one counter delta. That undercounts VolumeCount and/or EphemeralEvictedCount in GetLedgerStats.
Non-blocking but should be fixed in this PR:
openapi.ymlstill documentsTemplateUsage.countasint64, while the proto/Go/JSON surface isfixed64/uint64. Generated clients can use the wrong signed type.- The
LedgerStatsOpenAPI schema still describes onlyaccountCountandtransactionCount, but the HTTP DTO now returnstransactionCount,volumeCount,referenceCount,postingCount,logCount,ephemeralEvictedCount,transientUsedCount,revertCount, andnumscriptExecutionCount. docs/technical/architecture/subsystems/usage/usagebuilder.mdstill says a primary-store rollback/cursor-ahead situation triggersusagestore.Reset()on the next boot/tick, while the code explicitly documents that no rollback/cursor-ahead re-check is needed andReset()has no production caller.
Checks run locally:
git diff --check origin/release/v3.0...HEAD
GOROOT= go test ./internal/application/usagebuilder ./internal/storage/usagestore ./internal/adapter/http -run 'Test.*(Usage|Counter|Template|LedgerStats|Numscript|Store|Process)' -count=1
GOROOT= go test ./internal/application/usagebuilder ./internal/application/indexbuilder -run 'Test.*(Color|Volume|Purged|Ephemeral|Excluded|Dedup)' -count=1They pass, but the last command reports [no tests to run] for usagebuilder, so the colored-volume counter bug is currently uncovered.
…usagebuilder # Conflicts: # AGENTS.md # internal/infra/backup/rebuild.go # internal/infra/backup/rebuild_test.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Re-checking current head 023cb12.
I cannot approve this head yet. The skipped-order counter fixes and the usagebuilder documentation correction are still good, but the protobuf blocker remains: LedgerStats.metadata_count = 3 is removed from misc/proto/common.proto without reserving the field number and name.
This message is public API/protobuf surface. Please add both reservations inside message LedgerStats:
reserved 3;
reserved "metadata_count";then regenerate the protobuf outputs. Also note that this head still has NumaryBot review and codecov/patch failing, so it is not approval-ready independently of this blocker.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Re-checking current head 023cb12 with the pre-stable compatibility concern explicitly excluded.
I agree the LedgerStats.metadata_count proto-reservation point is only compatibility hygiene if no stable release has shipped, so I am no longer treating that as a blocker.
I still cannot approve this head because the current NumaryBot findings are real correctness issues, not compatibility issues:
-
extractPurgedVolumesbuilds exclusion keys from only(account, asset)while the read-index lookup path carries(account, asset, color). Colored purged/ephemeral volumes can miss the exclusion check, leaving stale account-to-transaction index entries; uncolored entries sharing the pair can also be over-excluded. -
usagebuilderper-audit volume deduplication keys also omitcolor, so two distinct color buckets for the same(ledger, account, asset)in one audit entry collapse into one counter delta. That can undercountVolumeCount/EphemeralEvictedCountfor colored volumes.
Those need fixing before approval. The remaining codecov/patch failure is separate and should be checked after the correctness fix.
…dup keys The color-of-money merge made TouchedVolume color-aware, but two consumers still keyed volume sets on (account, asset) only: - indexbuilder extractPurgedVolumes built the purged/ephemeral exclusion set without color, while isExcluded looks it up with the posting's color — colored evictions were never matched (stale index rows) and uncolored postings sharing the (account, asset) were wrongly excluded. Now mirrors domain.TouchedVolumeSet. - usagebuilder volumeSetKey omitted color, so two color buckets of one (account, asset) in a single audit entry deduped to one, undercounting VolumeCount / EphemeralEvictedCount. Both now carry color, matching the (account, asset, color) volume identity.
The LedgerStats schema declared only accountCount (required but never emitted) and transactionCount, while the handler serializes nine usage counters. Replace the properties with the nine fields the DTO actually returns (transactionCount, volumeCount, referenceCount, postingCount, logCount, ephemeralEvictedCount, transientUsedCount, revertCount, numscriptExecutionCount), drop accountCount, and update the endpoint + api-comparison descriptions.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The PR introduces the usagebuilder subsystem and event-counter cutover. Most previously raised issues have been addressed or formally resolved through documented design decisions (checker scope limited to primary Pebble store, LedgerBoundaries reserved field numbers, epoch timestamp fix, skip-gating for CreateTransaction/RevertTransaction). One concrete compilation-blocking issue remains: the removal of LedgerStats.metadata_count from the proto leaves an e2e test caller (tests/e2e/cluster/restore_test.go) that still calls the now-removed GetMetadataCount() accessor, which will cause that tagged test package to fail to compile.
Findings outside the diff
🟠 [major] Removed proto accessor GetMetadataCount() still called in e2e test — tests/e2e/cluster/restore_test.go
The LedgerStats.metadata_count field has been removed from misc/proto/common.proto, so the generated Go accessor GetMetadataCount() no longer exists. However, tests/e2e/cluster/restore_test.go still calls stats.GetMetadataCount(). This causes the e2e && s3 tagged test package to fail to compile, breaking CI for any run that includes those tags.
Suggestion: Remove or update the GetMetadataCount() call in restore_test.go to reflect the field removal. If the metric is still needed for the test assertion, use the appropriate replacement field or remove the assertion entirely.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1464 (comment)
… pipeline Adds table-driven tests feeding synthetic AuditEntry/AuditItem/Log/AppliedProposal fixtures through Builder.processAuditEntries against a real temp-dir usagestore, covering the previously-untested path above commitBatch: the create/revert/mirror dispatch fan-out, per-audit-entry volume dedup (shared account counted once; distinct colors counted independently — regression guard for the color-dedup fix), failed-proposal skip, LogSequence==0 skip, transient-volume tally, batch boundaries + cursor advance + idempotent EOF resume. Addresses the reviewer's no-coverage finding; matters because usagestore has no checker backstop.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The PR introduces the usagebuilder subsystem and event-counter cutover. The majority of previously raised concerns have been addressed: the checker/usagestore peer-secondary-store scope boundary is settled and merged via #1581, LedgerBoundaries proto field numbers 3–10 are now reserved, the stale-cursor-after-rollback scenario is handled by Reset() with self-healing on boot, CreateTransaction/RevertTransaction skip-inflation is fixed, and the metadata_count proto reservation deferral pre-GA is accepted with explicit rationale.
One architectural finding remains open at major severity: the archival-gap scenario where a primary-store checkpoint restore leaves a gap between the retained audit start and the current usagestore cursor. In this case the rollback guard does not fire (audit head is not behind the cursor), but the catch-up loop silently skips the gap and commits post-gap entries as progress, permanently producing incorrect usage projections. This is distinct from the rollback case fixed by Reset() and is not covered by the existing self-heal.
Several documentation issues were also identified: stale references to automatic Reset/rewind behaviour in usagebuilder.md and store.go, a stale rebuild-threshold comment in module.go, and an incorrect claim in AGENTS.md that the checker provides a backstop for CounterVolume drift — a checker pass that does not exist under the accepted peer-store design.
Findings outside the diff
🟠 [major] Archival gap silently skipped after primary-store checkpoint restore — internal/application/usagebuilder/builder.go
After a primary-store checkpoint restore whose retained audit range starts after the usagestore cursor, the rollback guard does not fire (the audit head is NOT behind the cursor), yet the catch-up loop silently skips the gap (cursor, firstAvailable) and commits the post-gap sequence as progress, permanently producing incorrect usage projections. This is materially different from the rollback/rewind scenario fixed by Reset() and is not covered by the existing self-heal.
Suggestion: Detect the archival-gap case (cursor < firstAvailableAuditEntry) separately from the rollback case. Either trigger a full usage-store rebuild (Reset + replay from the earliest available audit entry), clamp the cursor forward to firstAvailable with explicit zeroing of affected counters, or surface a health error that prevents the node from serving stale stats until an operator rebuild is completed.
🟡 [minor] Stale documentation references to removed Reset/rewind logic — internal/application/usagebuilder/usagebuilder.md (gfyrag, flemzord)
After the rollback/watermark code was reverted, several documentation references remain stale: usagebuilder.md (lines 113, 121, 129, 142) still claims Reset runs automatically on the next boot/tick, while counters.md and the actual code confirm there is no automatic rewind. Additionally, usagebuilder.md line 125 acknowledges that replay after archival undercounts VolumeCount, which contradicts any claimed rebuild completeness.
Suggestion: Update usagebuilder.md to accurately describe the current boot behaviour (cursor-forward-only, no automatic reset), remove or qualify the automatic-rewind claims, and add an explicit note that post-archival rebuilds may undercount VolumeCount.
🟡 [minor] Stale comment about usagebuilder rebuild threshold and rollback-catch-up race — internal/bootstrap/module.go
module.go still mentions that the usagebuilder shares the audit indexer's rebuild threshold and handles a rollback-then-catch-up race, but usagebuilder.NewBuilder() no longer takes a threshold and the reset/rewind logic has been removed.
Suggestion: Update the comment in module.go to reflect the current NewBuilder() signature and the actual boot behaviour.
🟡 [minor] Stale Reset() documentation describing automatic rollback detection — internal/storage/usagestore/store.go
The store.go Reset() method is documented as being triggered automatically when the builder detects a primary-store rollback, but there is no automatic production caller; Reset() is only called from tests and potentially from explicit operator tooling.
Suggestion: Update the Reset() doc comment to accurately state that it is an operator-triggered or test-only mechanism, not an automatically invoked self-heal path.
🟡 [minor] AGENTS.md claims non-existent checker backstop for CounterVolume drift — AGENTS.md
AGENTS.md states that a silent volume-preload miss that inflates usagestore.CounterVolume would 'eventually be surfaced by the checker's volume-count pass'. However, the PR design and the rationale accepted via #1581 explicitly place usagestore counters outside checker scope. This misleads future reviewers and operators into relying on a checker pass that does not exist.
Suggestion: Remove the reference to a checker backstop for CounterVolume drift, or replace it with an accurate description of the actual recovery path (operator-triggered rebuild-usage).
Summary: #1464 (comment)
paul-nicolas
left a comment
paul-nicolas
left a comment
There was a problem hiding this comment.
Multi-model review — PR #1464 (usagebuilder + counter cutover)
Reviewed by Claude (3 area-split reviewers) and Codex (codex-cli), findings synthesized and each one adversarially verified against the exact PR revision (023cb124) before posting. Note: reviewers worked from gh pr diff since the local worktree sits one commit ahead of the pushed PR.
Posted inline (8): 2 Major, 6 Medium. No Critical.
Verification outcome — 12 Critical/Major/Medium candidates entered adversarial verification; 8 survived (5 confirmed as-is, 3 severity-adjusted), 4 filtered out:
- REJECTED
readLoghot-only under-count (was Major, flagged by both models) — refuted: the archived-log case is unreachable. Audit entries and their logs are purged atomically in the same chapter purge, so the builder never iterates an entry whose log is gone; thelog == nilbranch is a defensive guard. Good example of verification catching a shared false positive. - REJECTED
RebuildAlldouble-count (was Medium) — noRebuildAllexists in usagebuilder; reviewer conflated it withindexbuilder.RebuildAll. - REJECTED batch
DeleteLedgercoalescing wipe (was Medium) — premise is backwards:commitBatchenqueues range-deletes FIRST, then Puts, and the delete+recreate-in-one-batch case is explicitly handled. - DEMOTED
LedgerLog.new_kept_volumes"unverified projection" (was Major) — accurate that no checker pass reads it while its two siblings do, but it's an already-documented, accepted peer-store carve-out: the volume rows it summarizes are independently verified bycompareVolumes, and its only consumer is the out-of-scope usagestore. Moved to Minor below.
Codex vs Claude conflicts resolved:
- Checkpointed
GetLedgerStatszeros — Codex Major, Claude treated it as documented/acceptable -> settled at Medium (deliberate + documented, but a real contract regression worth an explicit decision). LedgerStatsmissingreserved 3— Codex Medium, Claude Minor -> Medium (wire-compat + consistency withLedgerBoundaries).- Rollback self-heal — Claude Medium, Codex Major -> Major (verifier confirmed the documented
Reset()self-heal is entirely unwired).
Minor / Nit (not posted inline)
internal/infra/state/write_set.go/internal/application/check/checker.go—LedgerLog.new_kept_volumesis a new primary-store field with no checker pass while its siblingspurged_volumes/ephemeral_volumesare folded intocompareExclusionProjections. Accepted carve-out (peer-store only), but worth recording explicitly under invariant #8 "Known projection gaps".internal/adapter/grpc/server_bucket.go:571—GetTemplateUsageomits thereq.GetLedger() == "" -> ErrLedgerNameRequiredguard every sibling RPC has; raw gRPC callers get a 404 instead of InvalidArgument.internal/application/ctrl/controller_default.go—GetTemplateUsagelacks thehistoricalshort-circuit thatGetLedgerStatshas; latent (no checkpoint path wired today) but will serve live data on a historical clone once one is.internal/application/usagebuilder/builder.go—registerMetrics()error (~87) andReadLastAuditSequenceerror (~195) are silently swallowed;Stop()nil-derefsb.wifStart()never ran.internal/application/usagebuilder/process_audit.go— empty-ledgerLedgerScopedorder silently skipped (invariant #7: should fail loud);readLoglog == nildefensive guard returns empty rather than failing loud (unreachable by design, low value); committed Pebble batch neverClose()d (pool churn); steady-state tail holds the main-store read RLock until EOF (unbounded vs the catch-up budget — could delayRestoreCheckpoint).internal/domain/processing/processor_transaction.go(~95) — dead_ = isNumscript(the var is still used at the producer selection above); comment listsrevert_count/reference_count, whichprocessCreateTransactionnever maintained.cmd/ledgerctl/ledgers/stats.go— CLI no longer printsMetadata; confirmdocs/ops/cli.md/ recorded demos reflect the change.
No emojis. Peer review — challenge anything you disagree with.
|
|
||
| // tick runs one steady-state iteration: refresh the audit-head gauge, then | ||
| // drain any pending audit entries from the persisted cursor forward. | ||
| // |
There was a problem hiding this comment.
[Major] Documented rollback self-heal (usagestore.Reset() + replay-from-0) is not wired into the code.
usagestore.Store.Reset() exists and is unit-tested, and the PR's own design docs promise it "fires on the next boot/tick ... on rollback detection" (see docs/.../usage/*.md and the diff's rollback table). But nothing in builder.go ever calls Reset(): boot() computes "gap": int64(pebbleLast) - int64(cursor) (line ~172), logs it, and then unconditionally calls processAuditEntries(cursor, ...) — a negative gap (cursor ahead of the audit head, i.e. the primary store was restored/rolled back beneath the cursor) triggers no wipe. tick() here even asserts "No rollback/cursor-ahead re-check is needed ... there is nothing to rewind", directly contradicting the docs. On a genuine restore-behind-cursor, ReadAuditEntries(cursor+1) hits EOF and the loop no-ops, leaving the projection permanently over-counted.
Fix: on boot (and/or tick), if cursor > pebbleLast, call usageStore.Reset(), zero the atomics, and replay from sequence 0 — the mechanism the docs already describe — or, if the scenario is truly impossible by design, delete the Reset() plumbing and the docs that reference it so code and docs agree.
There was a problem hiding this comment.
Wired in e85936115. boot() and tick() now call a shared resetIfRolledBack(cursor, auditHead) helper: when the persisted cursor sits ahead of the audit head (primary store restored beneath it), it calls usageStore.Reset(), rewinds the lastProcessedAuditSeq atomic, and returns cursor 0 so the fold replays from scratch — the mechanism the docs describe. The stale tick() comment ("no rollback re-check is needed") was replaced. Added TestResetIfRolledBack (wipe-and-rewind + at/behind-head no-op).
| @@ -1672,7 +1682,6 @@ message PreparedQueryCursor { | |||
| message LedgerStats { | |||
| fixed64 transaction_count = 1; | |||
There was a problem hiding this comment.
[Major] Removing LedgerStats.metadata_count breaks the e2e build via an un-diffed call site.
Deleting metadata_count from LedgerStats (and regenerating) removes commonpb.LedgerStats.GetMetadataCount(). tests/e2e/cluster/restore_test.go:786 still calls Expect(stats.GetMetadataCount()).To(Equal(uint64(0))) — that file is not part of this PR, so the tests/e2e/cluster package fails to compile under the e2e tag and takes the whole e2e suite down in CI. Every other consumer (stats.go, handlers_get_ledger_stats.go, controller_default.go, tests/e2e/business/stats_test.go) was scrubbed; this one was missed.
Fix: remove/replace line 786 in tests/e2e/cluster/restore_test.go, and grep -rn GetMetadataCount --include='*.go' (excluding generated *.pb.go) to confirm no other call site remains before merge.
There was a problem hiding this comment.
Fixed in e85936115 (restore_test.go:786 GetMetadataCount assertion dropped). One nuance: this file is //go:build e2e && s3, and the standard CI gate (just test-e2e-coverage) builds with -tags e2e only, so it was not actually red in CI — but test-e2e-full would have failed, so worth fixing regardless. While there I also migrated the pre-existing color-of-money breakage in the same s3 files (GetVolumes()["USD"] map access + AccountVolume.GetInput/GetOutput -> FindVolume(asset,"")), which shipped on release/v3.0 from #1438 and is unrelated to this PR, so e2e,s3 now compiles clean.
| fixed64 transaction_count = 1; | ||
| fixed64 volume_count = 2; | ||
| fixed64 metadata_count = 3; | ||
| fixed64 reference_count = 4; |
There was a problem hiding this comment.
[Medium] LedgerStats tag 3 (metadata_count) deleted without a reserved.
The fields jump volume_count = 2 -> reference_count = 4, leaving tag 3 an unreserved gap. LedgerBoundaries in raft_cmd.proto correctly does reserved 3..10 with names; LedgerStats should get the same treatment. LedgerStats is a gRPC response type, so during a rolling upgrade an old node could still emit tag 3 and a future field reusing tag 3 (or the name metadata_count) would mis-decode.
Fix: add reserved 3; and reserved "metadata_count"; to the LedgerStats message.
There was a problem hiding this comment.
Holding on this one: we are intentionally NOT adding reserved for LedgerStats tag 3 pre-GA — same line we took on the ErrorReason enum and elsewhere in this cycle. There is no production cluster and no persisted LedgerStats (it is a transient gRPC/HTTP response type, never stored), so there is no wire-compat contract to preserve during a rolling upgrade of alpha environments that are wipe-able. Adding reserved/deprecated shims we would carry only for hypothetical old emitters is exactly the pre-GA compat code we are avoiding (coherence over proto-stability). If a compat need appears at GA we will reserve then. Note the contrast with LedgerBoundaries (which IS persisted in Pebble, hence its reserved 3..10).
|
|
||
| // volumeSetKey mirrors state.volumeSetKey — kept local to avoid crossing | ||
| // package boundaries just for a triple of strings. | ||
| type volumeSetKey struct { |
There was a problem hiding this comment.
[Medium] volumeSetKey drops the color dimension, mis-counting CounterVolume for multi-color accounts.
Volume identity on this branch is per (ledger, account, asset, color) — state.volumeSetKey (write_set_new_volumes.go) carries Color, the emitted commonpb.TouchedVolume carries Color, and the checker's AccountAssetKey keys on color. But this local volumeSetKey is only {ledger, account, asset} (the comment "mirrors state.volumeSetKey" is now false — it dropped the 4th field), and applyVolumeAnnotations dedups new-kept/purged/ephemeral tuples on that colorless key within an audit entry (lines 410/424/434). When one (account, asset) has two distinct colors newly created (or evicted) in the same FSM batch, the second color is deduped away, so CounterVolume moves by 1 instead of 2 (and CounterEphemeralEvicted likewise). The old write_set_counters.countVolumeDeltas counted per full color-distinguished key, so this is a regression, business-visible via GetLedgerStats.VolumeCount, and — since usagestore is outside checker scope — never surfaced.
Fix: add a color field to volumeSetKey and populate it from v.GetColor() at 410/424/434; fix the comment.
There was a problem hiding this comment.
Already fixed in aae177682 (before this review round): volumeSetKey now carries color and the three applyVolumeAnnotations sites populate it with v.GetColor(), so distinct color buckets of one (account, asset) dedup independently. 2e92c8bdf adds an integration test (distinct_colors_same_account_count_independently) that pins it.
| // This is the same trade-off that keeps read-index rebuild scoped to | ||
| // live state — future work can add checkpointed usage projections if | ||
| // clients need them. | ||
| if ctrl.historical { |
There was a problem hiding this comment.
[Medium] Checkpointed GetLedgerStats now silently returns zeroed usage counters.
For historical (checkpoint-scoped) reads the method returns &stats with only TransactionCount/LogCount set; every usage-derived counter (PostingCount, RevertCount, NumscriptExecutionCount, ReferenceCount, EphemeralEvictedCount, TransientUsedCount, VolumeCount) stays at zero because the usagestore isn't checkpoint-aware. Before this PR those counters lived on the checkpoint-consistent LedgerBoundaries, so a checkpoint-scoped stats read returned real historical values; now it returns a struct mixing real IDs with fabricated zeros. The in-code comment documents this as intentional, but the response contract (openapi) doesn't say checkpoint reads zero these fields, and silently returning 0 for a ledger with millions of postings is a surprising regression.
Fix: prefer an explicit signal over fabricated data — either reject checkpointed GetLedgerStats with Unimplemented until a checkpointed usage projection exists, or document the zeroing in the contract (openapi) so clients can distinguish "0" from "not available at this checkpoint".
There was a problem hiding this comment.
Addressed by documenting the contract rather than fabricating data or dropping the useful fields. e85936115 adds a note to the openapi /stats endpoint (and the in-code comment) that on a checkpoint-scoped read only transactionCount/logCount are checkpoint-consistent and the usage-derived counters are returned as 0 (usagestore is not checkpoint-aware) — so a client reads 0 on a checkpoint as "not available at this checkpoint" rather than a real value. Chose this over Unimplemented to preserve the checkpoint-accurate TransactionCount/LogCount; happy to switch to Unimplemented if you would rather the endpoint refuse checkpoint reads outright.
| @@ -10,7 +10,6 @@ import ( | |||
| type ledgerStatsJSON struct { | |||
| TransactionCount uint64 `json:"transactionCount"` | |||
There was a problem hiding this comment.
[Medium] Stats response reshaped but openapi.yml LedgerStats schema is stale.
This PR re-sources and re-shapes GET /v3/{ledgerName}/stats: ledgerStatsJSON now returns transactionCount, volumeCount, referenceCount, postingCount, logCount, ephemeralEvictedCount, transientUsedCount, revertCount, numscriptExecutionCount and no longer returns metadataCount or accountCount. But openapi.yml (LedgerStats, ~line 3663) still declares only accountCount (required) + transactionCount. Per CLAUDE.md "update openapi.yml when HTTP endpoints change", the schema must match the DTO.
Fix: update the LedgerStats schema to list the nine counters actually returned, drop accountCount, and do not add metadataCount.
There was a problem hiding this comment.
Already fixed in ee4c99811: the LedgerStats schema now lists the nine counters actually serialized, accountCount dropped from properties/required, metadataCount not re-added, endpoint description + api-comparison rows updated.
| 7. **Never silently skip a "should not happen" branch** — A branch that is reachable only if an invariant is violated (nil where the contract says non-nil, a state we believe unreachable, a cache miss after a guaranteed preload, etc.) MUST surface a loud signal: `return fmt.Errorf("invariant: ...")` so it bubbles up, or `assert.Unreachable(...)` for SUT-level invariants exercised under antithesis. A silent `return nil` / `continue` on these branches hides real bugs — particularly catastrophic in the FSM apply path, where a no-op desyncs nodes from each other. Branches that represent genuine runtime conditions (cache miss as an expected outcome, stale proposal, deleted entity) keep their soft `return nil`. The distinction is whether the case is *expected* (soft skip OK) or *impossible by design* (must fail loudly). The comment must say *why* the case is impossible so a reader can decide whether to add a hard fail or relax the rule. | ||
|
|
||
| 8. **The audit log is the only source of truth — every other persisted dataset is a projection and must be verified by the checker** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. Everything else stored in Pebble — `Log`, `AuditItem`, `AppliedProposal`, `LedgerLog.PurgedVolumes`, attribute caches (`Volume`, `Metadata`, `Transaction`, `Reference`, `Boundary`, etc.), reversion bitsets, idempotency keys, mirror cursors, chapters, bloom filters, signing keys, the read-side index — is a *projection* of orders that already live in the audit chain. Projections are rebuildable from the audit on demand, so we deliberately do NOT extend the hash chain to cover them (refactor over hash binding — see `feedback_audit_is_source_of_truth`). In exchange, **`internal/application/check/checker.go` MUST verify every projection it persists**: re-derive the value the projection should hold by replaying the audit (`ReplayLedgerLog`, `SimulateEphemeralPurge`, `partitionVolumes`, etc.) and compare to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. A projection that the checker does not verify is a tampering vector — adding a new persisted projection without a matching compare* / collect* pass in the checker is the violation. Two scope refinements: (a) this applies to the **primary FSM store** — the single store `Check()` opens and walks; **peer secondary stores** (the `readstore` inverted index today; the `usagestore` counters forthcoming with EN-1334) are out of *main-store checker* scope *by construction* since `Check()` never opens them. That scope carve-out is **not** a claim they are integrity-safe: the readstore serves READY indexes directly to business-visible queries with no scan fallback, and its automated detect/drop/rebuild is **not yet wired** (tracked under `EN-1323`), so a corrupted or tampered index is a **current open integrity gap on the peer-store side** — a per-replica rebuild-health concern of the index builder, not an invariant-#8 main-store concern. Out of the main-store checker's mandate, not out of every integrity concern. (b) A primary-store projection may be exempted from a dedicated pass only when it is either (i) deterministically rebuildable from still-retained verified state through a real, *wired* rebuild path — not rebuildable merely in principle — or (ii) purely informational and intentionally carried across restore (cf. `BuildStatus`). These are distinct bases: of the *Known projection gaps* below, `DefaultEnforcementMode` and `SubAttrLedgerMetadata` qualify under (i) via `RebuildDelta`, while `EphemeralEvictedCount` / `TransientUsedCount` qualify under (ii) — they are NOT rebuilt by `RebuildDelta`, so the wired-rebuild basis must not be claimed for them. The current passes are `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus), and `compareMirrorV2LogID` (stored `LedgerBoundaries.last_mirror_v2_log_id` **==** `max(audited MirrorIngest.v2_log_id)` per ledger — a full equality check, `CHECK_STORE_ERROR_TYPE_MIRROR_V2LOGID_MISMATCH` on ANY divergence; the FSM enforces a contiguous applied prefix so at rest the two must be exactly equal, and the baseline floor is seeded from archived `Boundary` rows now included in the compact baseline snapshot; pre-field clusters are unsupported, no backfill leniency), and `compareSchema` (per-ledger `LedgerInfo.MetadataSchema` vs the replayed CreateLedger.initial_schema + SetMetadataFieldType/RemovedMetadataFieldType logs; under archived chapters the boundary schema is seeded from the baseline checkpoint, which carries LedgerInfo for this purpose), and `compareAccountTypes` (per-ledger `LedgerInfo.AccountTypes` vs the replayed AddAccountType/RemoveAccountType logs, baseline-seeded under archiving like the schema), and `compareLedgerPresence` (the live ledger set in the store must match the audit-derived set both ways: every audit-live ledger — CreateLedger with no later DeleteLedger, or a non-deleted baseline ledger under archiving — must have a *live* stored `LedgerInfo` else `MISSING_LEDGER` (covers an entry deleted outright OR tampered to a soft-deleted tombstone, which the compareSchema/compareAccountTypes loops skip), and every live stored `LedgerInfo` must be audit-backed else `UNAUDITED_LEDGER` (an injected empty ledger row carries no schema/types for the projection passes to flag). The audit-derived set is NEVER seeded from the live store — under archiving it comes from the baseline checkpoint, keeping the check independent of the data it verifies), and `compareReferences` (the SubAttrReference reference→txID uniqueness index vs the references replayed from CreatedTransaction/RevertedTransaction logs, baseline-seeded under archiving; verified both ways — missing, unaudited, and retargeted rows are all flagged; rows for deleted or cleanup-pending ledgers are skipped since they legitimately linger until a covering purge runs deleteLedgerData), and `compareBoundaries` (per-ledger `LedgerBoundaries` vs the checker’s re-derivation: NextTransactionId/NextLogId/PostingCount/RevertCount from the replayed logs plus the chain-bound AuditItem order effects — mirror fill-gap advances and NumscriptExecutionCount live on the orders, not the ledger-log stream — baseline-seeded under archiving; VolumeCount/MetadataCount/ReferenceCount are compared against a recount of the stored attribute rows they summarize, which their own passes verify entry-by-entry, so the chain is rows↔audit, counter↔rows; EphemeralEvictedCount/TransientUsedCount are intentionally excluded — informational, carried across restore, cf. BuildStatus), and `compareReversions` (the per-ledger reversion bitsets — `ZonePerLedger`/`SubPLReversions`, the projection the FSM's already-reverted gate reads — vs the reverted set derived from baseline tx-row markers plus the replayed RevertedTransaction logs; exact equality both ways — a lost bit re-admits a double revert, an unaudited bit blocks a legitimate one; driven purely by audit-derived state — no persisted marker (pending-cleanup included) can exempt a live ledger, stored rows for non-live ledgers are flagged since DeleteLedger deletes them at apply on both the live path and the replay, and undecodable rows are reported); extend the list as new persisted projections land. **Known projection gaps**: `LedgerBoundaries.EphemeralEvictedCount` / `TransientUsedCount` are excluded from `compareBoundaries` and carried across restore rather than re-derived — informational counters (cf. BuildStatus); `LedgerInfo.DefaultEnforcementMode` and the ledger-metadata attribute (`SubAttrLedgerMetadata`) have no compare pass yet (both are rebuilt on restore by RebuildDelta). | ||
| 8. **The audit log is the only source of truth — every other persisted dataset is a projection and must be verified by the checker** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. Everything else stored in Pebble — `Log`, `AuditItem`, `AppliedProposal`, `LedgerLog.PurgedVolumes`, attribute caches (`Volume`, `Metadata`, `Transaction`, `Reference`, `Boundary`, etc.), reversion bitsets, idempotency keys, mirror cursors, chapters, bloom filters, signing keys, the read-side index — is a *projection* of orders that already live in the audit chain. Projections are rebuildable from the audit on demand, so we deliberately do NOT extend the hash chain to cover them (refactor over hash binding — see `feedback_audit_is_source_of_truth`). In exchange, **`internal/application/check/checker.go` MUST verify every projection it persists**: re-derive the value the projection should hold by replaying the audit (`ReplayLedgerLog`, `SimulateEphemeralPurge`, `partitionVolumes`, etc.) and compare to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. A projection that the checker does not verify is a tampering vector — adding a new persisted projection without a matching compare* / collect* pass in the checker is the violation. Two scope refinements: (a) this applies to the **primary FSM store** — the single store `Check()` opens and walks; **peer secondary stores** (the `readstore` inverted index today; the `usagestore` counters forthcoming with EN-1334) are out of *main-store checker* scope *by construction* since `Check()` never opens them. That scope carve-out is **not** a claim they are integrity-safe: the readstore serves READY indexes directly to business-visible queries with no scan fallback, and its automated detect/drop/rebuild is **not yet wired** (tracked under `EN-1323`), so a corrupted or tampered index is a **current open integrity gap on the peer-store side** — a per-replica rebuild-health concern of the index builder, not an invariant-#8 main-store concern. Out of the main-store checker's mandate, not out of every integrity concern. (b) A primary-store projection may be exempted from a dedicated pass only when it is either (i) deterministically rebuildable from still-retained verified state through a real, *wired* rebuild path — not rebuildable merely in principle — or (ii) purely informational and intentionally carried across restore (cf. `BuildStatus`). These are distinct bases: of the *Known projection gaps* below, `DefaultEnforcementMode` and `SubAttrLedgerMetadata` qualify under (i) via `RebuildDelta`, while `EphemeralEvictedCount` / `TransientUsedCount` qualify under (ii) — they are NOT rebuilt by `RebuildDelta`, so the wired-rebuild basis must not be claimed for them. The current passes are `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus), and `compareMirrorV2LogID` (stored `LedgerBoundaries.last_mirror_v2_log_id` **==** `max(audited MirrorIngest.v2_log_id)` per ledger — a full equality check, `CHECK_STORE_ERROR_TYPE_MIRROR_V2LOGID_MISMATCH` on ANY divergence; the FSM enforces a contiguous applied prefix so at rest the two must be exactly equal, and the baseline floor is seeded from archived `Boundary` rows now included in the compact baseline snapshot; pre-field clusters are unsupported, no backfill leniency), and `compareSchema` (per-ledger `LedgerInfo.MetadataSchema` vs the replayed CreateLedger.initial_schema + SetMetadataFieldType/RemovedMetadataFieldType logs; under archived chapters the boundary schema is seeded from the baseline checkpoint, which carries LedgerInfo for this purpose), and `compareAccountTypes` (per-ledger `LedgerInfo.AccountTypes` vs the replayed AddAccountType/RemoveAccountType logs, baseline-seeded under archiving like the schema), and `compareLedgerPresence` (the live ledger set in the store must match the audit-derived set both ways: every audit-live ledger — CreateLedger with no later DeleteLedger, or a non-deleted baseline ledger under archiving — must have a *live* stored `LedgerInfo` else `MISSING_LEDGER` (covers an entry deleted outright OR tampered to a soft-deleted tombstone, which the compareSchema/compareAccountTypes loops skip), and every live stored `LedgerInfo` must be audit-backed else `UNAUDITED_LEDGER` (an injected empty ledger row carries no schema/types for the projection passes to flag). The audit-derived set is NEVER seeded from the live store — under archiving it comes from the baseline checkpoint, keeping the check independent of the data it verifies), and `compareReferences` (the SubAttrReference reference→txID uniqueness index vs the references replayed from CreatedTransaction/RevertedTransaction logs, baseline-seeded under archiving; verified both ways — missing, unaudited, and retargeted rows are all flagged; rows for deleted or cleanup-pending ledgers are skipped since they legitimately linger until a covering purge runs deleteLedgerData), and `compareBoundaries` (per-ledger `LedgerBoundaries` vs the checker’s re-derivation: only NextTransactionId/NextLogId are verified here — derived from the replayed logs plus the chain-bound AuditItem order effects (mirror fill-gap advances live on the orders, not the ledger-log stream), baseline-seeded under archiving; the mirror high-water `last_mirror_v2_log_id` is verified separately by `compareMirrorV2LogID`. The per-ledger usage counters — `PostingCount`, `RevertCount`, `NumscriptExecutionCount`, `VolumeCount`, `MetadataCount`, `ReferenceCount` — no longer live on `LedgerBoundaries`: they moved to the `usagestore` peer secondary store (EN-1334) and are out of main-store checker scope **by construction** (per the peer-store carve-out in scope refinement (a) above), their integrity being a peer-store rebuild-health concern rather than an invariant-#8 main-store concern; `EphemeralEvictedCount`/`TransientUsedCount` remain on `LedgerBoundaries` but are intentionally excluded — informational, carried across restore, cf. BuildStatus), and `compareReversions` (the per-ledger reversion bitsets — `ZonePerLedger`/`SubPLReversions`, the projection the FSM's already-reverted gate reads — vs the reverted set derived from baseline tx-row markers plus the replayed RevertedTransaction logs; exact equality both ways — a lost bit re-admits a double revert, an unaudited bit blocks a legitimate one; driven purely by audit-derived state — no persisted marker (pending-cleanup included) can exempt a live ledger, stored rows for non-live ledgers are flagged since DeleteLedger deletes them at apply on both the live path and the replay, and undecodable rows are reported); extend the list as new persisted projections land. **Known projection gaps**: `LedgerBoundaries.EphemeralEvictedCount` / `TransientUsedCount` are excluded from `compareBoundaries` and carried across restore rather than re-derived — informational counters (cf. BuildStatus); `LedgerInfo.DefaultEnforcementMode` and the ledger-metadata attribute (`SubAttrLedgerMetadata`) have no compare pass yet (both are rebuilt on restore by RebuildDelta). |
There was a problem hiding this comment.
[Medium] Invariant #8 now self-contradicts on EphemeralEvictedCount / TransientUsedCount.
This PR reserves LedgerBoundaries fields 3-10 (which includes ephemeral_evicted_count = 7 and transient_used_count = 8), deletes their only writer (write_set_counters.go), and reads them from usagestore (controller_default.go -> CounterEphemeralEvicted/CounterTransientUsed). Yet the rewritten invariant #8 still says these two counters "remain on LedgerBoundaries but are intentionally excluded — informational, carried across restore", repeats it in "Known projection gaps", and lists them under refinement (b)(ii). That is factually wrong post-cutover and, since #8 governs checker integrity, would mislead a maintainer.
Fix: state that EphemeralEvictedCount/TransientUsedCount moved to usagestore (out of main-store scope like the other usage counters); remove them from the "Known projection gaps" list and the (b)(ii) example.
There was a problem hiding this comment.
Fixed in e85936115. Invariant #8 now states EphemeralEvictedCount/TransientUsedCount moved to the usagestore peer store (LedgerBoundaries reserves tags 3-10), out of main-store checker scope like the other usage counters; removed them from the "Known projection gaps" list and from the refinement (b)(ii) example (BuildStatus stays the (ii) example).
| @@ -0,0 +1,64 @@ | |||
| package http | |||
There was a problem hiding this comment.
[Medium] New /numscripts/{name}/usage endpoint lacks handler-level and e2e coverage.
The tests here only exercise the pure mapper toTemplateUsageJSON. The HTTP handler handleGetNumscriptUsage itself is untested: empty name -> 400 INVALID_REQUEST, backend-error propagation via handleError, 404 for unknown/soft-deleted ledger, and the writeOK envelope shape. The sibling handlers_get_ledger_stats_test.go (edited in this PR) drives its handler end-to-end through MockBackend + httptest, so this endpoint is below the established bar. There is also zero end-to-end coverage (grep GetTemplateUsage tests/ pkg/ is empty) — no pkg/actions helper and no tests/e2e/business spec asserting count/lastUsed increment after a real invocation or the 404-vs-zero contract.
Fix: add handler tests mirroring TestHandleGetLedgerStats_* (empty name, propagated error, success) and an e2e spec that invokes a templated numscript and asserts the counters.
There was a problem hiding this comment.
Added in dceaa24f5. Handler tests now drive handleGetNumscriptUsage end-to-end via MockBackend + httptest, mirroring TestHandleGetLedgerStats_*: success envelope, never-invoked → zero, missing {name} → 400 INVALID_REQUEST (gomock proves the backend is never called), missing {ledgerName} → 400, unknown ledger → 404, generic backend error → 500, no-leader → 503, plus a full-route integration request. Added pkg/actions.GetTemplateUsage and an e2e spec (tests/e2e/business/numscript_usage_test.go) that registers + invokes a template and polls the async usagebuilder counter with Eventually (no sleep), asserting count/lastUsed increment and the unknown-template→zero vs unknown-ledger→404 contract.
…dings - builder: wire the documented usagestore.Reset() rollback self-heal. boot() AND tick() now detect a primary-store rollback beneath the usage cursor (cursor > audit head) via a shared resetIfRolledBack helper, wipe the projection + cursor, and replay from 0 — previously the docs promised this but no code path called Reset(), leaving the projection permanently over-counted on a restore-behind-cursor (no checker backstop for the peer store). Add a focused test. - AGENTS.md invariant #8: EphemeralEvictedCount/TransientUsedCount moved to usagestore (LedgerBoundaries reserves tags 3-10) — correct the stale text that said they 'remain on LedgerBoundaries'. - openapi + controller: document that checkpoint-scoped /stats returns 0 for usage-derived counters (not checkpoint-aware) so a client reads 0 as 'unavailable at this checkpoint'. - e2e(s3) restore tests: migrate the missed color-of-money volume API (Volumes[asset] -> FindVolume(asset,"")) and drop the removed LedgerStats.GetMetadataCount assertion, unbreaking the e2e,s3 build.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
All previously raised blocker and major findings have been resolved or formally accepted as design decisions prior to this consolidation pass. The stale-cursor-after-rollback blocker (both the boot-time momentary-compare race and the runtime steady-state variant) is addressed by the rewindIfCursorAhead + gapExceedsThreshold boot guard and the per-tick cursor-ahead check, with regression tests in place. The checker/usagestore scope boundary is settled by design via #1581: usagestore is a rebuildable peer side-store outside checker scope, with integrity ensured by the cryptographically-verified audit chain and ledgerctl store rebuild-usage. The CreateTransaction/RevertTransaction skip-inflation issues are fixed. The LedgerStats.metadata_count proto reservation is intentionally deferred pre-GA, with explicit rationale. The LedgerBoundaries field-number reservation and the NewKeptVolumes checker gap are closed or accepted by design with documentation in counters.md. Two minor/nit-level documentation inconsistencies remain: stale rollback/rewind references in code comments that survived the watermark-code revert, and an AGENTS.md invariant that incorrectly implies a checker backstop for CounterVolume that does not exist under the accepted peer-store contract. Neither blocks merging.
Findings outside the diff
⚪ [nit] Stale rollback/rewind references in documentation after revert — general
Several documentation files still reference the reset/rewind logic and rebuild threshold that was reverted: internal/bootstrap/module.go mentions the usagebuilder sharing the audit indexer's rebuild threshold and catching a rollback-then-catch-up race, and internal/storage/usagestore/store.go documents Reset() as being triggered by rollback detection. These are no longer accurate after the revert of the watermark code.
Suggestion: Update the stale comments/docs in bootstrap/module.go and usagestore/store.go to reflect the current (post-revert) boot logic and remove references to the rebuild threshold wiring and rollback-driven Reset() that no longer exist in the codebase.
⚪ [nit] AGENTS.md claims checker backstop for CounterVolume that does not exist — AGENTS.md
The volume-preload invariant in AGENTS.md states that a silent volume-cache miss inflating usagestore.CounterVolume would "eventually be surfaced by the checker's volume-count pass". However, the accepted peer-store design (confirmed in #1581 and the PR discussion) explicitly keeps usagestore counters outside checker scope. This stale claim will mislead future operators and reviewers into relying on a checker guard that does not exist.
Suggestion: Remove or reword the claimed checker backstop in AGENTS.md to match the accepted side-store contract: usagestore is a rebuildable peer side-store whose integrity is a rebuild-health concern, not a checker-verified primary-store projection.
Summary: #1464 (comment)
Closes the reviewer gap where only the pure mapper toTemplateUsageJSON was tested. Adds handler tests driving handleGetNumscriptUsage end-to-end via MockBackend + httptest (success, never-invoked zero, missing name/ledger 400, unknown ledger 404, backend error 500, no-leader 503, full-route integration), a pkg/actions.GetTemplateUsage helper, and an e2e spec that registers + invokes a template and polls the async usagebuilder counter (Eventually, no sleep), asserting the unknown-template-returns-zero vs unknown-ledger-404 contract.
4 participants
Summary
usagebuildersubsystem materialises per-Numscript-template invocation counters (count + lastUsed) from the FSM audit chain into a dedicated peer secondary Pebble store (usagestore). Exposed viaGET /v3/{ledger}/numscripts/{name}/usage.LedgerBoundariesintousagestore. This is the full cutover: the event counters (posting, revert, numscript_execution, reference) AND the attribute-derived counters (Volume, EphemeralEvicted, TransientUsed) now live inusagestore;MetadataCountis dropped for now (no sound foundation — see below). The FSM increment sites are removed and the proto fields deleted + reserved.LedgerStatssources every migrated counter fromusagestore.Architecture notes
CreatedTransactionpayload doesn't carryNumscriptReference— the raw order inAuditItem.SerializedOrderdoes. When a posting count is needed (revert / script-backed create) the specific log is fetched viaReadLogBySequence— a single point read per item on hot Pebble cache.LedgerLog—NewKeptVolumes(+1),PurgedVolumes(−1),EphemeralVolumes(0, tracked forEphemeralEvictedCount) — split this way (vs. the earlier 2-list encoding) to keep ephemeral-heavy workloads from paying 2× bytes on the log payload (EN-1422).[0xFE][0x01]. LogCommitted notification wakes the loop; the audit chain advances in the same batch as the log. If the primary store is rolled back beneath the cursor, the builder self-heals: it wipes the projection and replays from sequence 0 (usagestore.Store.Reset()on boot).usagestoreopens its own Pebble instance at<data-dir>/usage/(peer toreadstore). Rebuild = drop the directory (orReset()); the builder replays from cursor=0.feedback_constructor_injectionin project memory), and the existingsignal.FanOutis extended to a 4th target.Cutover semantics (EN-1420 / EN-1422)
On production upgrade, every migrated counter resets to 0 and repopulates from the earliest audit entry still reachable in Pebble. Historic pre-upgrade counter values are lost. This is the trade-off — accepted by product to avoid a stateful migration. A one-time seed from
LedgerBoundarieswas explicitly considered and rejected because theLedgerBoundariescounters were themselves inaccurate on any ledger with pre-existing history.processor_transaction.go,processor_revert_transaction.go,processor_mirror.go, andwrite_set_counters.go.LedgerBoundaries(reserved 3..10+ names) — the vtprotobuf unmarshaller retains unknown-field bytes and re-emits them on MarshalVT, so the tags must never be reused.GetLedgerStatsreads posting / revert / numscript_execution / reference / ephemeral_evicted / transient_used / volume fromusagestore.GetCounter. Missing key = 0. Checkpoint-scoped (historical) reads short-circuit before touching the usagestore and return only the boundary-derivedTransactionCount+LogCount.MetadataCountis intentionally not returned. The admission preload no longer injects old metadata values, so the FSM-side counter can no longer distinguish "new key" from "overwrite". It is disabled until it can be rebuilt on a sound foundation (open question, no ticket yet).Test plan
just pre-commit— 0 issues (verified locally)GOROOT= go test ./internal/...— all pass locally (verified)GOROOT= go build ./...— clean (verified)usagestore/store_test.gocovers round-trip Get/Put + DeleteLedger cascade +Reset;usagebuilder/process_audit_test.gocovers batchState aggregation, applyDelta, mergeTemplateUsage, timestampGreater./v3/{ledger}/numscripts/{name}/usageand inGetLedgerStats.ledgerctl store rebuild-usage --data-dir <dir>drops and rebuilds the store.LedgerStatsvalues on a ledger with existing history — expect the migrated counters to be lower than pre-cutover values (that's the reset).Files
internal/storage/usagestore/,internal/application/usagebuilder/internal/adapter/http/handlers_get_numscript_usage.go,cmd/ledgerctl/store/rebuild_usage.gomisc/proto/common.proto(+TemplateUsage),misc/proto/bucket.proto(+RPC + request),misc/proto/raft_cmd.proto(LedgerBoundaries fields 3–10 deleted + reserved)internal/bootstrap/module.go,internal/application/ctrl/controller.go,internal/application/ctrl/controller_default.go,internal/adapter/grpc/{server,client}_bucket.go,internal/bootstrap/controller_routed.gointernal/domain/processing/processor_{transaction,revert_transaction,mirror}.go,internal/infra/state/write_set{,_counters,_new_volumes,_ephemeral_purge}.gointernal/application/check/checker.go(exclusion projection now foldsEphemeralVolumesalongsidePurgedVolumes).golangci.yaml,misc/devenv/monitoring-dashboards/jsonnet/lib/metrics.libsonnetopenapi.yml,docs/technical/contributing/api-comparison.md,docs/ops/cli.md,docs/technical/architecture/subsystems/usage/Follow-ups
MetadataCount— decide how to rebuild it on a sound foundation (the preload no longer carries old metadata values). No ticket yet.PebbleLastAuditSequenceobservability once accuracy on truncated-audit ledgers is understood.